feat(core): add portable document ingestion contracts - #1178
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5810e4dca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e5810e4 to
5a0ad61
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a0ad615fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5a0ad61 to
cbd903c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbd903c254
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cbd903c to
fca36b5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fca36b555a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
fca36b5 to
fa57ffe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa57ffe030
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
fa57ffe to
bc13b3d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc13b3d444
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bc13b3d to
61d7b0d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d7b0dd55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d7b0dd55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87b60bde48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Following this contract with interest — the One question before #135 has been open since June 2025 asking for this at the note level — section and observation anchors, so a quote can cite its source precisely. Its first example was a That blocker looks smaller now than it did then. Compared to last year, observations now carry their own permalinks — #1178 is where it becomes load-bearing. I don't have the standing to say what fits your architecture, so I asked Claude to sketch options merely as suggestions. Flagging that explicitly — these are prompts, not recommendations, and I'd defer to your read on all of them of coures:
My own thoughts--once parsed, can it store a page number and line number per page? That nested structure could potentially support direct quote architecture? EDIT (claude helped me understand the constraints better): Related thought — if a span could carry the source checksum alongside its location, a citation becomes verifiable rather than just locatable: you could confirm a quote still matches the document it came from, and detect drift if the source is ever re-extracted. Given how much this contract already checksums, that might be closer to free than it sounds. - Claude Entirely possible this belongs after v1 ships, or that one of these cuts against something I can't see from outside. Mostly I wanted to raise it while the contract is still open rather than after it isn't. |
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
The 'Simplify document semantic parsing' commit removed the bm_parse_semantics opt-out that the PR #1178 specification promises, so raw extraction bodies containing '- [category] ...', hashtags, or [[wiki links]] were indexed as observations and relations straight from untrusted PDF text. The unresolved Codex P1 on #1178 flagged exactly this: test_raw_note_uses_normal_graph_semantics asserted the polluting behavior as correct. Restore the contract: - DocumentNoteFrontmatterV1 requires bm_parse_semantics and rejects any value that disagrees with the ingestion stage (false for raw/failed, true for ready/needs_review). - DocumentIngestionRunFrontmatterV1 pins bm_parse_semantics to false. - enrich_document_markdown re-enables semantics only for the bounded enriched output. - EntityParser honors the opt-out solely for an explicit scalar false (bool or normalized string), keeping body content for search and ignoring non-scalar values without crashing. Replace test_raw_note_uses_normal_graph_semantics with the original test_raw_search_only_note_does_not_mint_graph_semantics assertions, restore the non-scalar safety test, and add a paired regression test proving the assembled raw note parses with zero observations and relations through the real EntityParser while the identical body without the opt-out does mint them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4 Signed-off-by: phernandez <paul@basicmachines.co>
87b60bd to
5af61b3
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@calebjpicker — on the page/section locator for
The reason it's not in this PR: a real locator wants the source page/line map to exist to resolve against, and that lives on the document/run provenance side ( I'll open a follow-up issue for "optional source locator on document observations (page → context, resolve against the extraction page map)" and link #135 so it's tracked rather than lost. Thanks for pushing on this while the contract was still open. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af61b359a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #1178: a source path like `C:/outside/report.pdf` (or the drive-relative `C:x`) passes PurePosixPath's is_absolute() check, but the derived sidecar stays drive-qualified and FileService joins it with base_path via `base_path / entity.file_path`, discarding the project root and materializing the note outside the project. Reject a PureWindowsPath drive or rooted form as well, matching the note-move path validation. Refs #1178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4 Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac09a17e34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ng them Codex review on #1178: the source and output path fields used NonEmptyText (strip_whitespace=True), so a path with leading/trailing whitespace was silently rewritten by the model, while the public derive_document_note_path validates the raw value and would reject it — so the accepted provenance and derived sidecar could diverge and a padded path could collide with another object's note. Use a non-stripping path type and reject leading/trailing whitespace in the canonical validator, matching AGENTS.md's fail-fast rule against silent rewrites. Refs #1178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4 Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34c4e9254d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nt contracts Two more Codex findings on #1178: - The raw-extraction Markdown body accepted NUL bytes; assemble preserves them and the hosted path writes to Postgres note_content (TEXT), which rejects NUL before EntityParser's cleanup. Reject NUL at the body validation boundary. - Source keys like `docs/CON.pdf` (reserved device name) or `docs/report.pdf:stream` (NTFS alternate data stream) passed path validation but never materialize as portable sidecars on Windows. Reject reserved device-name components and colon-based streams. Refs #1178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4 Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b1e63612
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review on #1178: beyond reserved names and colons, a POSIX source path can still carry components Windows cannot represent — the illegal characters <>:"|?* or a component ending in a dot or space (Windows strips those). The derived sidecar can't materialize under that exact name, so canonical note storage can't converge. Reject them in the project-relative path validator. Refs #1178 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017STCpbNsYjZgUdftxgEAZ4 Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 971459f036
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def derive_document_note_path(source_file_path: str) -> str: | ||
| """Return the portable sidecar path for one source file (for example ``report.pdf.md``).""" | ||
| source_path = _validate_project_relative_path(source_file_path) | ||
| return f"{source_path}.md" |
There was a problem hiding this comment.
Reject sidecar names that exceed filesystem limits
When the source's final component is a valid 253–255-byte filename on an ext4-backed project, this unconditional suffix produces a 256–258-byte sidecar name. Validation accepts the path, but materialization then fails permanently with ENAMETOOLONG, so the accepted document cannot converge to its portable file representation; validate the derived component length, including the .md suffix.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one. It's technically real but out of scope for this PR and mis-scoped to the schema layer:
- It requires a source filename within ~2 bytes of the OS maximum (253–255 bytes); no realistic source file is named that way.
- Core's own note-write path (write_note → file_service) has no filename-length check either, so a long note title hits the identical ENAMETOOLONG. Adding a bespoke length guard to the document contract alone holds it to a stricter standard than the base note format it feeds into.
- A hardcoded 255-byte check isn't portable: NAME_MAX is 255 bytes on ext4 but 255 UTF-16 units elsewhere and larger on other filesystems, and total-path limits differ. The only place the real limit is knowable is the file layer at write time.
- It fails loudly (ENAMETOOLONG) rather than corrupting or converging to a wrong state; note_content is retained and still serves reads.
If we want NAME_MAX handling, it should be one shared guard in the file layer covering write_note and document ingestion together — tracked separately, not bolted onto this schema. Leaving this open would just add a non-portable check that the rest of the system doesn't enforce.
Why
Hosted document ingestion needs one portable Basic Memory contract for generated Markdown, provenance, deterministic identity, and safe semantic parsing. Without that boundary, Core and Cloud would disagree about what a raw extraction note means, how retries converge, and which fields an enrichment agent may control.
This PR is the Core contract used by basic-memory-cloud#1650, implements the parser-neutral portion of #1006, and deliberately leaves historical revision indexing to #1177.
Specification
Scope
Core must provide strict, provider-neutral contracts for:
type: document;type: document_ingestion_run;Behavioral contract
source,extraction,ingestion, anddocumentmetadata is strict, nested, extra-forbidden, and serializable through either the YAMLschemaalias or the Pythonschema_reffield name.bm_parse_semantics: false; Core preserves body content for search while suppressing observation/relation parsing only for an explicit scalarfalse.Non-goals
Acceptance Criteria
documentanddocument_ingestion_runhave strict versioned Pydantic contracts.What Changed
src/basic_memory/schemas/document.py.src/basic_memory/schemas/__init__.py.document_ingestionnote-object source support.EntityParserto honor the raw semantic opt-out safely without dropping body content.Implementation Details
Testing
Automated
just fast-check: passed Ruff fix/check, formatting, andtytype checking.uv run pytest -q tests/schemas/test_document.py tests/markdown tests/test_runtime.py::TestRuntimeContracts::test_note_object_metadata_parses_safe_values_only: 128 passed;src/basic_memory/schemas/document.pyreached 100% statement coverage.uv run pytest -q --no-cov tests/cli/test_cli_exit.py::test_bm_cli_import_does_not_load_heavy_stack: 1 passed.61d7b0dd55313076a081bad823eb69796d4b8426: unit, SQLite integration, Postgres integration, semantic, Milvus, static, CodeQL, DCO, and CLA jobs passed or were skipped as designed.git diff --check: passed.Manual
Risks / Follow-ups
unknown_key, so this description does not claim GitHub Verified status.